⚠️ No Changeset found

Latest commit: 5afaa5e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

PR Review: fix(graphql-codegen-client-preset): handle Windows-style paths in WASM runtime

The fix is correct and well-reasoned. The root cause (WASM always using Unix path semantics while the host provides Windows paths) is clearly explained, and both normalization steps are necessary.

Positive aspects

• Removing the unused Path import is clean.
• Renaming the inner cwd to cwd_path avoids shadowing confusion.
• The is_absolute_path helper is well-documented and handles the WASM/Unix misclassification issue correctly.
• Snapshot test confirms the expected relative import path is generated correctly.

Issues

Minor: redundant .to_string() call

In get_relative_import_path:

let resolved_artifact_directory = if is_absolute_path(&artifact_directory) {
    artifact_directory.to_string()  // artifact_directory is already a String

artifact_directory is already a String, so .to_string() is a no-op clone. Either drop it (moving the owned value into the branch) or use artifact_directory.clone() to make the intent explicit. Since the else branch also consumes artifact_directory (via cwd_path.push(&artifact_directory)), you'd need artifact_directory.clone() in the if branch or restructure slightly, e.g.:

let resolved_artifact_directory = if is_absolute_path(&artifact_directory) {
    artifact_directory
} else {
    let mut cwd_path = PathBuf::from(&cwd);
    cwd_path.push(&artifact_directory);
    cwd_path.to_string_lossy().into_owned()
};

Suggestion: add a test for relative Windows paths

The new test covers Windows absolute paths (C:\...), but the backslash normalization also affects relative paths. A test with a relative artifact_directory containing backslashes (e.g. src\\gql) would give fuller coverage of the normalization logic and guard against regression.

Out of scope / known limitation: UNC paths

Windows UNC paths (\\server\share\path) are not handled by is_absolute_path. This is likely acceptable for now, but worth noting in a comment near the helper to avoid future confusion.

Overall this is a solid, focused fix. Addressing the redundant .to_string() and adding a relative-path test would round it out nicely.

@claude fix ci

Claude finished @kdy1's task in 3m 28s —— View job

CI Fixed

• Check CI status
• Identify failure: main branch added naming_convention field to GraphQLCodegenOptions; this PR's new get_windows_path_visitor test function didn't include it, causing a compile error in the CI merge commit
• Add naming_convention: "change-case-all#pascalCase" to get_windows_path_visitor in tests.rs
• Push fix (commit 5afaa5e)

The CI failure was a merge conflict in test code — main added a naming_convention field to GraphQLCodegenOptions while this PR was open, but the new get_windows_path_visitor function added by this PR didn't include it. Added the missing field to fix compilation.
| Branch